-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
introduce code checker and code cleanup #171
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mavimo
changed the title
introduce unused check that report unused variable or functions
introduce unused & gosimple code check
Aug 15, 2018
mavimo
force-pushed
the
introduce-unused-check
branch
from
August 15, 2018 20:13
a4b40f9
to
a299de8
Compare
mavimo
changed the title
introduce unused & gosimple code check
introduce code checker and code cleanup
Aug 15, 2018
mavimo
force-pushed
the
introduce-unused-check
branch
from
August 15, 2018 21:02
56bcdbd
to
d98e5d3
Compare
@xetys I'll rebase it in few hours, maybe you can start to check and indicate me if there is something wrong that I can fix after rebase :) |
I merged some of your PRs, but we have now a little conflict here 😄 |
That's a huge PR and I'm sure it's all good with it. However I will spend some more time on this one before merging, at least to learn what I was doing wrong 😄 |
…s that are used in a single specific case
mavimo
force-pushed
the
introduce-unused-check
branch
from
August 23, 2018 19:12
d98e5d3
to
5b57ac6
Compare
mavimo
force-pushed
the
introduce-unused-check
branch
from
August 23, 2018 19:21
5b57ac6
to
37126dc
Compare
xetys
approved these changes
Aug 25, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi @xetys @JohnnyQQQQ I do a bit of refactoring (I start with small change but than "degenerate" in a large amount of changes, sorry :|).
I try to have separate commit so you can check commit-by-commit instead doing a large code review (each commit is described below).
Before you start to check let me explain a bit motivation that move me to refactor the code:
I will make clear and "less error prone" use this library so I start to improve code with code checkers (
unused
,gosimple
,mispell
,unparam
andstaticcheck
), each commit introduce a checker than the related fixes to make it green:unused
code check (see ca4a535)unused
(see 909e551)gosimple
code checker (see 733cf88)gosimple
(see a7bdad0)mispell
code checker (see aa1f8ea)mispell
(see 8578ef7)unparam
code checker (see 2342234)staticcheck
code checker (see 32c35a4)staticcheck
(see 7c12a0b)Codeclimate report some issue on duplicated code or code too complex to use, after some investigation I start to clean the code and remove duplicate:
hetzner-go
package and various clanup (see c7b8919)There are some function that require "correlate invokation" (eg after we create the cloud priovider we should call init method and than set the cloud init string. This is an anti-pattern since a user do not know that we should always invoke 3 function in this sequence each time. The next step was related to factory "helper" function that generate two object from different packages, most of the time thant we ignore the second object from the codebase, so I prefer to remove helper and make clear what/when we need to create a cloud provider and cloud manager:
The last part of this refactoring is related to the "utils" we have. Most of the time utils are bad file since include a potpurri of function that do not have a well defined scope and make harder to understand it. Having public method in utils expose method from package that overcomplicate the public surface. At last a package should not panic but need to return errors and eventually command use this error to panic.
I hope that I not misunderstand the current codebase and the changes are good for this codebase.
PS: after this and other PRs I do today I'm planning a bit of refactoring also on addon (I think that "fail" is not a good idea, and the
addon.Install()
method should return anderror
, than we can fail in command; and command (moving to a structure like what drone/drone does since using init method should create issue to other library that try to include some package from it, maybe we should discuss it before start to develop it?PS2: I forgot to mention that I tested (using hetzner cloud provider E2E) create a key, create a cluster, add a worker, get kubectl config, list ssh key, list ssh cluster, delete the cluster; if there are other tests that you think should be done let me know.
PS3: the last commit (see 37126dc) contains post rebase fixes